Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NetBSD support. #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add NetBSD support. #27

wants to merge 2 commits into from

Conversation

zoulasc
Copy link
Contributor

@zoulasc zoulasc commented Jan 15, 2025

No description provided.

@compudj
Copy link
Contributor

compudj commented Jan 15, 2025

Please add a Signed-off-by tag to the commit message.

@zoulasc
Copy link
Contributor Author

zoulasc commented Jan 15, 2025

I did

git commit --amend --signoff 

and then

git push --force

Is that what I need to do?

@compudj
Copy link
Contributor

compudj commented Jan 15, 2025

Yes, thanks! Also, can you confirm that you're run the userspace-rcu "make regtest" and "make short_bench" without issues ? Listing the testing done in the commit and adding a Tested-by tag would help.

@zoulasc
Copy link
Contributor Author

zoulasc commented Jan 16, 2025

Added, all tests pass.

Signed-off-by: Christos Zoulas <[email protected]>
Tested-by: Christos Zoulas <[email protected]>

regtest.out:
============================================================================
Testsuite summary for userspace-rcu 0.15.0
============================================================================
TOTAL: 852
PASS:  852
SKIP:  0
XFAIL: 0
FAIL:  0
XPASS: 0
ERROR: 0
============================================================================
TIME=38:58.39 CPU=597.9% (13605.556u 375.860s) SWAPS=0 (157727+3641680)pf (0i+338o) (1Kc+32Kd)

short_bench.out:
============================================================================
Testsuite summary for userspace-rcu 0.15.0
============================================================================
TOTAL: 852
PASS:  852
SKIP:  0
XFAIL: 0
FAIL:  0
XPASS: 0
ERROR: 0
============================================================================
TIME=1:02:33.91 CPU=716.0% (26438.855u 439.871s) SWAPS=0 (166007+2996145)pf (0i+353o) (0Kc+9Kd)
@compudj
Copy link
Contributor

compudj commented Jan 16, 2025

OK, I'll merge your patch, but I would recommend implementing futex_async and futex_noasync specifically for NetBSD, as was done for OpenBSD and FreeBSD (see include/urcu/futex.h). For instance, FreeBSD uses _umtx_op(). This will be more efficient than the busy-wait and pthread_condvar fallback implementations.

Considering that this will affect public headers installed on the system, it is a matter of ABI compatibility. It would therefore be simpler to introduce an efficient futex support along with the commit re-introducing NetBSD support before a release is done.

Thanks!

Tested-by: Christos Zoulas <[email protected]>

Add futex implementation for NetBSD

regtest
============================================================================
Testsuite summary for userspace-rcu 0.15.0
============================================================================
 TOTAL: 852
 PASS:  852
 SKIP:  0
 XFAIL: 0
 FAIL:  0
 XPASS: 0
 ERROR: 0
============================================================================
TIME=23:04.34 CPU=1153.2% (15409.920u 554.945s) SWAPS=0 (3296+3746659)pf (0i+222o) (1Kc+30Kd)

short_bench
============================================================================
Testsuite summary for userspace-rcu 0.15.0
============================================================================
============================================================================
TIME=45:40.15 CPU=1250.5% (33948.806u 318.658s) SWAPS=0 (953+3130345)pf (0i+285o) (0Kc+6Kd)
@zoulasc
Copy link
Contributor Author

zoulasc commented Jan 16, 2025

Added the futex implementation and ran the tests again. I am not sure if the tests actually called the futex code though. How do I make sure?

@compudj
Copy link
Contributor

compudj commented Jan 17, 2025

Added the futex implementation and ran the tests again. I am not sure if the tests actually called the futex code though. How do I make sure?

A quick way to make sure that your code is actually compiled is to add

#error "some message"

in the code.

A quick way to make sure that your code is actually run within the testsuite is to add an abort() in your code.

@zoulasc
Copy link
Contributor Author

zoulasc commented Jan 17, 2025

It is compiled and it is called :-) You can commit it.

rokuyama pushed a commit to IIJ-NetBSD/netbsd-src that referenced this pull request Jan 17, 2025
@compudj
Copy link
Contributor

compudj commented Jan 17, 2025

I notice a few problems with your latest commit:

  • it should be rebased on userspace RCU master (which has your first commit merged),
  • the patch title should be the first line. Signed-off-by should be at the end of the commit message.
  • I notice you used a futex() implementation. Posts like this one: https://mail-index.netbsd.org/tech-kern/2023/12/11/msg029334.html make me wonder if this compatibility layer is well tested. Is there an equivalent to the freebsd umtx as well on netbsd ? If yes, then it might be more robust to implement the wait/wakeup support based on umtx than the futex compatibility layer ?

@zoulasc
Copy link
Contributor Author

zoulasc commented Jan 17, 2025

  1. I will rebase it
  2. I will amend the commit message
  3. We don't have the equivalent of FreeBSD umtx, and I don't think that this system call is well tested (although there are unit tests https://nxr.netbsd.org/xref/src/tests/lib/libc/sys/t_futex_ops.c), since it is not used elsewhere (for example: https://nxr.netbsd.org/xref/src/sys/external/bsd/compiler_rt/dist/lib/sanitizer_common/sanitizer_linux.cc#660 is not using it). The code was added for linux system call compatibility. Perhaps we should not be using it in userspace rcu either. I will ask what other people think

@zoulasc
Copy link
Contributor Author

zoulasc commented Jan 17, 2025

Another issue is that there is currently no sparc (non v9), sh3, and vax support (I will check if there are more missing)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants